Make no_mangle on foreign items explicit instead of implicit #144678
Conversation
rustbot
added
A-attributes
|
Some changes occurred in compiler/rustc_codegen_ssa |
|
Can you please squash the last two commits? |
This comment has been minimized.
This comment has been minimized.
|
Can do |
jdonszelmann
force-pushed
the
no-mangle-extern
branch
2 times, most recently
from
1790424 to
5240989
Compare
jdonszelmann
force-pushed
the
no-mangle-extern
branch
from
5240989 to
e75b93d
Compare
jdonszelmann
force-pushed
the
no-mangle-extern
branch
from
e75b93d to
9003a3b
Compare
| @@ -192,6 +192,10 @@ impl CodegenFnAttrs { | |||
| /// * `#[export_name(...)]` is present | |||
| /// * `#[linkage]` is present | |||
| /// | |||
| /// Note that this returns true for foreign items. | |||
| /// However, in some places that care about `contains_extern_indicator`, foreign items | |||
| /// (in an `extern` block) should explicitly be ignored. | |||
There was a problem hiding this comment.
Is it possible to make this not return true for foreign items? Foreign items don't need to be exported and the point of this method is to check if an item needs to be exported.
There was a problem hiding this comment.
it's annoying since we don't have a did or TyCtxt here. We could, but that'd involve adding another flag in the codegen fn attributes. I decided against that but it's perfectly possible.
|
@bors r+ |
bors
added
S-waiting-on-bors
…jorn3 Make no_mangle on foreign items explicit instead of implicit for a followup PR I'm working on I need some foreign items to mangle. I could add a new attribute: `no_no_mangle` or something silly like that but by explicitly putting `no_mangle` in the codegen fn attrs of foreign items we can default it to `no_mangle` and then easily remove it when we don't want it. I guess you'd know about this r? `@bjorn3.` Shouldn't be too hard to review :) Builds on rust-lang#144655 which should merge first.
Rollup of 13 pull requests Successful merges: - #143857 (Port #[macro_export] to the new attribute parsing infrastructure) - #144070 (Implement `hash_map` macro ) - #144322 (Add lint against dangling pointers from local variables) - #144667 (`AlignmentEnum` should just be `repr(usize)` now) - #144678 (Make no_mangle on foreign items explicit instead of implicit) - #144790 (Multiple bounds checking elision failures) - #144794 (Port `#[coroutine]` to the new attribute system) - #144805 (compiletest: Preliminary cleanup of `ProcRes` printing/unwinding) - #144808 (`Interner` arg to `EarlyBinder` does not affect auto traits) - #144816 (Update E0562 to account for the new impl trait positions) - #144822 (Return a struct with named fields from `hash_owner_nodes`) - #144824 (Updated test links in compiler) - #144829 (Use full flag name in strip command for Darwin) r? `@ghost` `@rustbot` modify labels: rollup
|
Trying to diagnose rollup failure #144846 (comment) @bors try jobs=test-various |
Make no_mangle on foreign items explicit instead of implicit try-job: test-various
This comment has been minimized.
This comment has been minimized.
|
💔 Test failed (CI). Failed jobs:
|
|
@bors r- |
bors
removed
the
S-waiting-on-bors
|
@bors2 try jobs=test-various |
Make no_mangle on foreign items explicit instead of implicit try-job: test-various
|
@bjorn3 that seems promising? |
jdonszelmann
force-pushed
the
no-mangle-extern
branch
from
5aa8fb1 to
7aa8707
Compare
|
@bors2 try jobs=test-various |
This comment has been minimized.
This comment has been minimized.
Make no_mangle on foreign items explicit instead of implicit try-job: test-various
|
@bjorn seems ok? |
|
@rustbot ready |
rustbot
added
S-waiting-on-review
|
@bors r+ |
bors
added
S-waiting-on-bors
|
☀️ Test successful - checks-actions |
What is this?This is an experimental post-merge analysis report that shows differences in test outcomes between the merged PR and its parent PR.Comparing d9dba3a (parent) -> 8e62bfd (this PR) Test differencesShow 4 test diffs4 doctest diffs were found. These are ignored, as they are noisy. Test dashboardRun cargo run --manifest-path src/ci/citool/Cargo.toml -- \
test-dashboard 8e62bfd311791bfd9dca886abdfbab07ec54d8b4 --output-dir test-dashboardAnd then open Job duration changes
How to interpret the job duration changes?Job durations can vary a lot, based on the actual runner instance |
|
Finished benchmarking commit (8e62bfd): comparison URL. Overall result: ❌ regressions - please read the text belowOur benchmarks found a performance regression caused by this PR. Next Steps:
@rustbot label: +perf-regression Instruction countOur most reliable metric. Used to determine the overall result above. However, even this metric can be noisy.
Max RSS (memory usage)Results (primary -1.3%, secondary 2.4%)A less reliable metric. May be of interest, but not used to determine the overall result above.
CyclesResults (secondary 3.7%)A less reliable metric. May be of interest, but not used to determine the overall result above.
Binary sizeThis benchmark run did not return any relevant results for this metric. Bootstrap: 464.325s -> 465.671s (0.29%) |
|
sigh, ye that's significant I'm afraid :/ |
| @@ -443,6 +443,27 @@ fn apply_overrides(tcx: TyCtxt<'_>, did: LocalDefId, codegen_fn_attrs: &mut Code | |||
| if tcx.should_inherit_track_caller(did) { | |||
| codegen_fn_attrs.flags |= CodegenFnAttrFlags::TRACK_CALLER; | |||
| } | |||
|
|
|||
| // Foreign items by default use no mangling for their symbol name. | |||
| if tcx.is_foreign_item(did) { | |||
There was a problem hiding this comment.
I suspect the extra query call here is the cause for the perf regression.
for a followup PR I'm working on I need some foreign items to mangle. I could add a new attribute:
no_no_mangleor something silly like that but by explicitly puttingno_manglein the codegen fn attrs of foreign items we can default it tono_mangleand then easily remove it when we don't want it.I guess you'd know about this r? @bjorn3. Shouldn't be too hard to review :)
Builds on #144655 which should merge first.